images/server: Improvements to install-packages.sh #212
Conversation
|
I forgot to mention: with regards to the naming, since we've added long options I think it'd be OK to have alises if we want like |
|
Also also, since #210 got merged the script relies on build args and the build args map to the existing options, so I think both ought to be updated to match (probably in one PR, but not necessarily one patch) |
There was a problem hiding this comment.
I like the general direction this is going but I have some quibbles with the way the names a little confused WRT to plurals. But one issues is do we want to change all the variable names or is that too much of a pain.
I've made changes to indicate the plural nature both internally and for options.
I forgot to mention: with regards to the naming, since we've added long options I think it'd be OK to have alises if we want like --install-custom-repo and --custom-repos could store to the same variable IF it makes things clearer and easier to use.
I hope the rename to --install-custom-repos is good enough for now.
Also also, since #210 got merged the script relies on build args and the build args map to the existing options, so I think both ought to be updated to match (probably in one PR, but not necessarily one patch)
Can you please check the updated version?
We could avoid the need for `--custom-repos` optional argument by modifying install_custom_repo option to accept more than one space separated urls. Best reviewed with `git show -w`. Signed-off-by: Anoop C S <[email protected]>
Signed-off-by: Anoop C S <[email protected]>
anoopcs9
force-pushed
the
rm-custom-repos-arg
branch
from
5e2a347 to
f5b81d6
Compare
anoopcs9
changed the title
|
@anoopcs9 are you OK to merge this? Either one of us can follow up on the ceph repo option aspect in a follow up pr. Do you want to get a 2nd review? |
As we speak I've almost incorporated that pending change. I'll shortly update the PR. |
anoopcs9
force-pushed
the
rm-custom-repos-arg
branch
from
f5b81d6 to
747951b
Compare
mergify
bot
dismissed
phlogistonjohn’s stale review
Pull request has been modified.
anoopcs9
force-pushed
the
rm-custom-repos-arg
branch
from
747951b to
76183d3
Compare
mergify
bot
dismissed
phlogistonjohn’s stale review
Pull request has been modified.
|
Updated once again with two additional commits (not related to custom repo management). |
anoopcs9
force-pushed
the
rm-custom-repos-arg
branch
from
06b35ef to
8d27cde
Compare
mergify
bot
dismissed
phlogistonjohn’s stale review
Pull request has been modified.
Reduce the assumption level a bit by making it explicit that we need to install ceph from custom repos with the help of an extra option `--ceph-from-custom`. Signed-off-by: Anoop C S <[email protected]>
Signed-off-by: Anoop C S <[email protected]>
With package_selection set to `custom` for custom-repos source we failed to install libcephfs-proxy2, samba-vfs-cephfs and few other required packages due to non satisfying switch case down the line. Instead we rename package_selection to `custom-repos-devbuilds` so as to enter the relevant switch case to install crucial packages. Signed-off-by: Anoop C S <[email protected]>
Signed-off-by: Anoop C S <[email protected]>
EPEL repository is enabled by default after installation. Signed-off-by: Anoop C S <[email protected]>
There's a possibility of an infinite loop if the hashed repo name for the url happens to exist at the destination. Therefore replace `while` loop with basic `if` condition and skip the url altogether when hashed repo name exists. Signed-off-by: Anoop C S <[email protected]>
anoopcs9
force-pushed
the
rm-custom-repos-arg
branch
from
8d27cde to
9a48755
Compare
|
@Mergifyio refresh |
✅ Pull request refreshed |
|
mergify!! :shakingfist: |
|
Mergiyf is complaining:
Something to follow up with later as I am very disappointed in mergify lately. Manually merging now. |
phlogistonjohn
merged commit 4bbb290
into
samba-in-kubernetes:master
3 participants
--install-custom-reporather than new--custom-repos.--package-selection, by default we failed to install some important packages for custom package source.--enablerepo=epel.